update dasht-query-line and dasht-query-html to work with tsv#56
update dasht-query-line and dasht-query-html to work with tsv#56smackesey wants to merge 7 commits into
Conversation
sunaku
left a comment
sunaku
left a comment
There was a problem hiding this comment.
Thanks for this PR. I've requested some minor changes in the code review.
| # only in those whose names match *DOCSET*s, by calling dasht-query-exec(1) | ||
| # and emits the results in groups of lines, as described in "Results" below. | ||
| # However, if no results were found, this program exits with a nonzero status. | ||
| # and emits the results as TSV. However, if no results were found, this program |
There was a problem hiding this comment.
Please spell out "TSV" in prose: "as TSV" -> "in Tab Separated Vector (TSV) format"
| # ### Results | ||
| # | ||
| # Each search result is printed to stdout as a group of four lines of text: | ||
| # Each search result is printed to stdout as a tab-separated line with fields: |
There was a problem hiding this comment.
The fields are separated, not the line; so "as a line with 4 tab-separated fields".
| { $1 = $1 } # strip whitespace from key | ||
|
|
||
| $2 == "=" { | ||
| result[$1] = substr($0, index($0, $2) + length($2) + 1) |
There was a problem hiding this comment.
I would change the index($0, $2) call to length($1) + 1 for robustness (no problem if $1 contains =).
| } | ||
|
|
||
| /./ # reject any empty lines from input | ||
| # print TSV line |
There was a problem hiding this comment.
I would drop this (redundant) comment.
|
|
||
| /./ # reject any empty lines from input | ||
| # print TSV line | ||
| printf "%s\t%s\t%s\t%s\n", result["name"], docset, \ |
There was a problem hiding this comment.
I would add parentheses to the printf() call and list each argument on its own line.
|
Made the changes! |
sunaku
left a comment
sunaku
left a comment
There was a problem hiding this comment.
Thanks @smackesey! I found some time to experiment with your latest changes and I would suggest applying the changes in the attached patch file in addition to my code review feedback.
This PR is coming along nicely and we'll reach the finish line soon! 😅 Thanks for your patience.
|
Thanks! So I made the changes you listed, but I was unable to apply the patch. I've never applied a patch file to a git repo before so perhaps I was doing something wrong, but I tried |
sunaku
left a comment
sunaku
left a comment
There was a problem hiding this comment.
Thanks for the update. There's a potential AWK incompatibility that I've called out in the code review. No worries about the patch file: I'll apply it on my side when I finally merge the PR. 🤓
|
Looks great, thanks! I'll try this out for a few days and let you know. Overall, we should be good to merge. 👍 |
|
Hi @smackesey, I found an issue when searching the BASH docset:
Following the first result link shows a |
|
Fixed, the issue was that the |
|
Sorry for the long delay. 😅 I'll merge this PR soon (ETA this week). |
Switch to Tab-Separated Values (TSV) format in dasht-query-line(1) and dasht-query-html(1) for easier line-by-line parsing and user scripting.
|
Merged in commit 882c7f2. Thanks for your contribution 🙏 and sorry for the long delay! 😅 |
Switch to Tab-Separated Values (TSV) format in dasht-query-line(1) and dasht-query-html(1) for easier line-by-line parsing and user scripting.
2 participants
I extracted my work on TSV from the other Dash.app integration fork so that this could be separately merged. It follows your requests in #38.